Add 2-step verification for POST requests. #582

efeg · 2019-03-06T02:12:19Z

Addresses the issue #567.

Pending: Documentation on how to use two-phase verification.

...rc/main/java/com/linkedin/kafka/cruisecontrol/servlet/parameters/StopProposalParameters.java

sudoa

Thanks for the patch, left some comments.
One point about using UUID and/or session in the request that's related to 2-step verification.
When we want to check status of a request, can one just provide a reviewId?
Or if the user wants to check status of a old request with UUID in the header, will this request be handed off to purgatory?

...ontrol/src/main/java/com/linkedin/kafka/cruisecontrol/servlet/KafkaCruiseControlServlet.java

...java/com/linkedin/kafka/cruisecontrol/servlet/parameters/AddedOrRemovedBrokerParameters.java

...e-control/src/main/java/com/linkedin/kafka/cruisecontrol/servlet/purgatory/ReviewStatus.java

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/servlet/purgatory/Purgatory.java

...se-control/src/main/java/com/linkedin/kafka/cruisecontrol/servlet/purgatory/RequestInfo.java

sudoa · 2019-03-18T18:40:27Z

LGTM

kidkun

Thanks for the patch! Left some comments.

kidkun · 2019-03-18T19:07:08Z

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/servlet/purgatory/Purgatory.java

+ * The Purgatory is thread-safe.
+ */
+public class Purgatory implements Closeable {
+  private static final String FINAL_REASON = "Submitted approved request.";


we can move this FINAL_REASON into RequestInfo and simplify RequestInfo.submitReview(int reviewId, String reason) to RequestInfo.submitReview(int reviewId)

kidkun · 2019-03-18T19:22:43Z

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/servlet/purgatory/Purgatory.java

+    }
+
+    if (requestInfo.status() == ReviewStatus.SUBMITTED && LOG.isDebugEnabled()) {
+        LOG.info("Request {} has already been submitted (review: {}).", requestInfo.endpointWithParams(), reviewId);


LOG.info -> LOG.debug?

This is intentionally info level -- i.e. we would like to keep track of consecutive accesses -- removed && LOG.isDebugEnabled().

But it is gated by LOG.isDebugEnabled(), so this information will not get logged unless the verbose level is DEBUG?

Yes, we should not have had LOG.isDebugEnabled(), which was removed when I addressed the above feedback -- thanks for catching that.

kidkun · 2019-03-18T19:30:20Z

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/servlet/purgatory/Purgatory.java

+      }
+      // Apply review to each Request Info
+      ReviewStatus targetReviewStatus = entry.getKey();
+      requestIds.forEach(requestId -> _requestInfoById.get(requestId).applyReview(targetReviewStatus, reason));


How about we return only what is approved/discarded if approve /discard parameter is set for review endpoint, and return the whole list if no parameter it set?

kidkun · 2019-03-18T20:49:15Z

...ontrol/src/main/java/com/linkedin/kafka/cruisecontrol/servlet/parameters/ParameterUtils.java

+
+    Integer reviewId = Integer.parseInt(request.getParameter(parameterString));
+    // Sanity check: Ensure that if a review id is provided, no other parameter is in the request.
+    if (request.getParameterMap().keySet().size() != 1) {


request.getParameterMap().keySet().size() -> request.getParameterMap().size()

kidkun · 2019-03-18T21:35:29Z

...java/com/linkedin/kafka/cruisecontrol/servlet/parameters/AddedOrRemovedBrokerParameters.java

@@ -31,6 +31,7 @@
 *    &amp;json=[true/false]&amp;skip_hard_goal_check=[true/false]&amp;excluded_topics=[pattern]
 *    &amp;use_ready_default_goals=[true/false]&amp;verbose=[true/false]&amp;exclude_recently_demoted_brokers=[true/false]
 *    &amp;exclude_recently_removed_brokers=[true/false]&amp;replica_movement_strategies=[strategy1,strategy2...]
+ *    &amp;review_id=[id]


missing review_id=[id] for Decommission a broker section in document above.

kidkun · 2019-03-18T22:34:53Z

...java/com/linkedin/kafka/cruisecontrol/servlet/parameters/AddedOrRemovedBrokerParameters.java

@@ -31,6 +31,7 @@
 *    &amp;json=[true/false]&amp;skip_hard_goal_check=[true/false]&amp;excluded_topics=[pattern]
 *    &amp;use_ready_default_goals=[true/false]&amp;verbose=[true/false]&amp;exclude_recently_demoted_brokers=[true/false]
 *    &amp;exclude_recently_removed_brokers=[true/false]&amp;replica_movement_strategies=[strategy1,strategy2...]
+ *    &amp;review_id=[id]


Also I am thinking should we mention the fact that review_id parameter is exclusive with other parameter, put something like
POST /kafkacruisecontrol/add_broker?brokerid=[id1,id2...]&dryRun=[true/false]...
Or POST /kafkacruisecontrol/add_broker?review_id=[id]
in Javadoc.
(same apply to other POST endpoint parameters)

kidkun · 2019-03-18T22:50:25Z

...src/main/java/com/linkedin/kafka/cruisecontrol/servlet/response/PurgatoryOrReviewResult.java

+  }
+
+  private String getJSONString() {
+    List<Map<String, Object>> jsonRequestInfoList = new ArrayList<>();


initialize with size of _requestInfoById.size().

kidkun · 2019-03-18T23:22:59Z

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/servlet/UserTaskManager.java

@@ -295,10 +298,33 @@ synchronized void checkActiveUserTasks() {
    }
  }

+  private synchronized void removeFromPurgatory(UserTaskInfo userTaskInfo) {


I am wondering whether there is a reason why we keeps submitted items in purgatory until the task expires in UserTaskManager. looks to me it is enough that purgatory only maintains item in Pending and Approved status, once an item it discarded or submitted we can directly recycle it in purgatory.

Currently there are 2 place saves the information about approved request information, purgatory and UserTaskManager. If we want to know what tasks are approved, we can directly go to USER_TASK endpoint since we know if and only if a POST request is approved, it will be executed and we can get more information there(like execution result).

I thought a task can only be in UserTaskManager when it is approved?

Thanks for bringing up these points -- let me clarify:

Submitted tasks are in purgatory to ensure auditability, abstraction, and to ensure that if the request has already been submitted, the user is not attempting to create another user task with the same parameters and endpoint [added this sanity check in the latest commit]. The auditability lets us know details on an already executed request -- e.g. who submitted the request and when. The abstraction helps us filter out access to UserTasks in purgatory-level.

kidkun · 2019-03-20T19:13:06Z

...ontrol/src/main/java/com/linkedin/kafka/cruisecontrol/servlet/KafkaCruiseControlServlet.java

+    if (requestInfo.accessToAlreadySubmittedRequest()
+        && _userTaskManager.getUserTaskByUserTaskId(_userTaskManager.getUserTaskId(request), request) == null) {
+     throw new UserRequestException(
+         String.format("Attempt to start a new user task with an already submitted review. If you are trying to retrieve"


IIUC, this Exception message seems to be confusing under certain scenario.
Currently the default retention time for Purgatory is 14 days and the retention time for UserTaskManager is 1 day, so if a user want to retrieve (with UUID and reviewID) a task which was submitted 2 days, the user will get the this Exception and get confused(since he already provide the UUID).

Please see UserTaskManager#removeFromPurgatory, which removes request from purgatory if the task expires.

Let me know if I am missing something.

kidkun

LGTM!

efeg requested review from sudoa and kidkun March 6, 2019 02:12

sudoa reviewed Mar 6, 2019

View reviewed changes

...rc/main/java/com/linkedin/kafka/cruisecontrol/servlet/parameters/StopProposalParameters.java Outdated Show resolved Hide resolved

sudoa reviewed Mar 9, 2019

View reviewed changes

Add 2-step verification for POST requests.

4721643

efeg force-pushed the feat/2StepVerification branch from d8475df to 4721643 Compare March 13, 2019 18:55

Adem Efe Gencer and others added 2 commits March 13, 2019 23:51

Address the feedback.

ce35a41

Address the feedback and code cleanup.

435b159

kidkun suggested changes Mar 18, 2019

View reviewed changes

Address the feedback.

23e9ea3

kidkun reviewed Mar 20, 2019

View reviewed changes

kidkun approved these changes Mar 20, 2019

View reviewed changes

efeg merged commit d35af1b into linkedin:master Mar 20, 2019

efeg added a commit to efeg/cruise-control that referenced this pull request Mar 21, 2019

Add 2-step verification for POST requests. (linkedin#582)

77b83fd

efeg deleted the feat/2StepVerification branch March 21, 2019 00:40

efeg mentioned this pull request Mar 21, 2019

Add documentation on 2-step verification for POST requests #602

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 2-step verification for POST requests. #582

Add 2-step verification for POST requests. #582

efeg commented Mar 6, 2019

sudoa left a comment

sudoa commented Mar 18, 2019

kidkun left a comment

kidkun Mar 18, 2019

kidkun Mar 18, 2019

efeg Mar 20, 2019 •

edited

Loading

kidkun Mar 20, 2019

efeg Mar 20, 2019

kidkun Mar 18, 2019

kidkun Mar 18, 2019

kidkun Mar 18, 2019

kidkun Mar 18, 2019

kidkun Mar 18, 2019

kidkun Mar 18, 2019

sudoa Mar 19, 2019 •

edited

Loading

efeg Mar 20, 2019

kidkun Mar 20, 2019

efeg Mar 20, 2019

kidkun left a comment

Add 2-step verification for POST requests. #582

Add 2-step verification for POST requests. #582

Conversation

efeg commented Mar 6, 2019

sudoa left a comment

Choose a reason for hiding this comment

sudoa commented Mar 18, 2019

kidkun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efeg Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudoa Mar 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidkun left a comment

Choose a reason for hiding this comment

efeg Mar 20, 2019 •

edited

Loading

sudoa Mar 19, 2019 •

edited

Loading